fix: fix e2e tests #99
Conversation
| return encodeFunctionData({ | ||
| abi: parseAbi(["function approve(address spender, uint256 amount)"]), | ||
| args: [spender, amount], | ||
| }); | ||
| } |
There was a problem hiding this comment.
After writing the first version of this file, I've come to learn that viem provides a standard ERC20 ABI erc20Abi value within the library.
Wdyt about using the erc20Abi (source) value provided by viem instead of hardcoding the functions whenever we are calling some ERC20 function?
| for (const account of accounts) { | ||
| console.log(`Approving GRT txs on ${account.address} to ${horizonStaking}`); | ||
| // Stake GRT for other accounts | ||
| for (const account of otherAccounts) { |
There was a problem hiding this comment.
Feels like you could also include the service provider account here
| for (const account of otherAccounts) { | |
| for (const account of [...otherAccounts, serviceProvider]) { |
And remove the service provider code that's been repeated below solely for the serviceProvider account.
| if (receiptSetOp.status !== "success") { | ||
| throw new Error(`Transaction failed: setOperator for ${serviceProvider.address}`); |
| * | ||
| * @returns {Record<string, Caip2ChainId>} A mapping from lowercase bytes32 hash strings to their corresponding CAIP-2 chain IDs. | ||
| */ | ||
| export function generateChainIdHashMap(): Record<string, Caip2ChainId> { |
There was a problem hiding this comment.
Check Caip2Utils.findByHash, it might work for your use case here
| import { NullNotificationService } from "@ebo-agent/automated-dispute/src/index.js"; | ||
| import { RequestId, ResponseId } from "@ebo-agent/automated-dispute/src/types/index.js"; |
There was a problem hiding this comment.
Importing from a module's .js file
| @@ -102,6 +107,7 @@ describe.sequential("single agent", () => { | |||
| }); | |||
|
|
|||
| beforeEach(async () => { | |||
| Logger.getInstance(); | |||
There was a problem hiding this comment.
Is this something you had to do to avoid the tests crashing? Mind adding a comment explaining that?
| if (!accounts || accounts[0] === undefined || accounts[0]?.privateKey === undefined) { | ||
| throw new Error("Accounts not found"); | ||
| } |
There was a problem hiding this comment.
In which situation would the test reach this point with no accounts created? Wouldn't it fail before?
There was a problem hiding this comment.
If you kinda want to express that this test needs just the account[0] to be defined (as a pre-condition), an assert would convey the intention a little bit better here. Something like this:
assert(!accounts || accounts[0] === undefined, "Account not defined");
assert(accounts[0].privateKey, "Account has no private key");| console.log("Approving bond escalation module..."); | ||
| await protocolProvider.write.approveModule( | ||
| protocolContracts["BondEscalationModule"] as Address, | ||
| ); | ||
| console.log("Bond escalation module approved."); |
There was a problem hiding this comment.
Should this be inside some of the utility functions we have?
| if (requestCreatedEvent === undefined) { | ||
| throw new Error("RequestCreated event not found"); | ||
| } |
There was a problem hiding this comment.
Can the test reach this point with requestCreatedEvent being undefined? Wouldn't the expect right above it stop the test execution?
| if (badResponseProposedEvent === undefined) { | ||
| throw new Error("ResponseProposed event not found"); | ||
| } |
There was a problem hiding this comment.
You can:
| if (badResponseProposedEvent === undefined) { | |
| throw new Error("ResponseProposed event not found"); | |
| } | |
| expect(badResponseProposedEvent).toBeDefined() |
Right?
🤖 Linear
Closes GRT-253
Description
Remaining items